Skip to content

fix(integrations): declare kiro-cli multi-install safe#3477

Open
Quratulain-bilal wants to merge 3 commits into
github:mainfrom
Quratulain-bilal:fix/kiro-cli-multi-install-safe
Open

fix(integrations): declare kiro-cli multi-install safe#3477
Quratulain-bilal wants to merge 3 commits into
github:mainfrom
Quratulain-bilal:fix/kiro-cli-multi-install-safe

Conversation

@Quratulain-bilal

@Quratulain-bilal Quratulain-bilal commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

fixes #3471

kiro-cli uses a fully isolated .kiro/ root and .kiro/prompts command dir, a stable . separator with no dynamic paths, and a dedicated install manifest — it meets every documented multi-install-safe criterion. but multi_install_safe was never set on KiroCliIntegration, so co-installing it (e.g. alongside claude) left specify integration status permanently in ERROR (unsafe-multi-install) with no way to resolve it. --force bypasses the install-time gate but not the status finding.

fix: set multi_install_safe = True on KiroCliIntegration.

i verified the isolation rather than trusting the flag: the registry contract tests in tests/integrations/test_registry.py auto-parametrize over every declared-safe integration and check pairwise-disjoint agent roots, command dirs, and (via a real init+install) install manifests. with kiro-cli added they still all pass (365 tests), which empirically proves .kiro/ doesn't collide with any other safe integration.

also added an explicit multi_install_safe is True assertion in the kiro-cli test file (fails on the pre-fix code) and listed kiro-cli in the multi-install-safe docs table.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Declares Kiro CLI safe for co-installation and documents the behavior.

Changes:

  • Enables multi_install_safe for Kiro CLI.
  • Adds a regression assertion.
  • Adds Kiro CLI to the safety reference table.
Show a summary per file
File Description
src/specify_cli/integrations/kiro_cli/__init__.py Enables safe multi-installation.
tests/integrations/test_integration_kiro_cli.py Verifies the safety declaration.
docs/reference/integrations.md Documents Kiro CLI, but contains a safety-criteria inconsistency.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread docs/reference/integrations.md

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback

@Quratulain-bilal

Quratulain-bilal commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

thanks, you're right that the docs read like a contradiction — fixed the wording in 050446f.

digging in though, it's not a real collision. two separate layers were getting conflated:

  • the multi_install_safe flag governs only the install footprint: agent root, command dir, and manifest. kiro-cli's are .kiro/ + .kiro/prompts + its own manifest, disjoint from codex's .agents/ + .agents/skills. the registry contract tests (test_registry.py) enforce this pairwise across every safe key — adding the flag pulled kiro-cli in and all 389 still pass.
  • neither codex nor kiro-cli writes AGENTS.md as part of install. that mapping lives in agent-context-defaults.json and is owned by the optional agent-context extension.

and that extension can't actually make two safe integrations write the same file: in update-agent-context.sh it reads a single key — opts.get('integration') or opts.get('ai') from init-options.json — and seeds the context file for that one default integration only, never for every installed integration at once. so codex + kiro-cli co-installed still only seeds whichever one is the default.

so the flag is correct as-is; the docs just described a constraint (dedicated context file) that the flag doesn't govern. dropped that clause from the definition and added a note explaining what the Isolation column and the flag actually cover.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread docs/reference/integrations.md Outdated
@mnriem

mnriem commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

We need to resolve by remove the agent file mention altogether. By default integrations are now multi-install safe. The agent-context extension is not. Maybe we should call that out in its README. Can you make that change?

kiro-cli is declared multi_install_safe (it uses a fully isolated .kiro/
root, .kiro/prompts command dir, a stable '.' separator, and a dedicated
manifest — github#3471). main ended up with the flag assigned twice in the class
body; this drops the bare duplicate and keeps a single declaration with the
comment explaining why it's safe.

also adds the missing kiro-cli row to the multi-install-safe table in
docs/reference/integrations.md, and a test asserting the flag is set (the
registry contract tests already enforce the actual path isolation against
every other safe integration).
@Quratulain-bilal
Quratulain-bilal force-pushed the fix/kiro-cli-multi-install-safe branch from 050446f to 2f9ffd5 Compare July 17, 2026 00:01
@mnriem
mnriem requested a review from Copilot July 17, 2026 19:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread tests/integrations/test_integration_kiro_cli.py Outdated
@mnriem

mnriem commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback

per maintainer guidance on github#3477: the Isolation table listed each
integration's agent-context file (AGENTS.md, CLAUDE.md, etc.) alongside
its command dir, which contradicted the safety definition (Copilot flagged
kiro-cli/codex both mapping to AGENTS.md). those context files are owned by
the optional agent-context extension, not by multi_install_safe — the flag
governs only the command directory + manifest.

- renamed the column to 'Command directory' and removed the agent-file
  entries, so it lists only what each integration actually manages
- reworded the definition to note agent-context is a separate concern and
  is not multi-install safe
- removed the duplicate test_declared_multi_install_safe (the existing
  test_declares_multi_install_safe already asserts the same thing)
@Quratulain-bilal

Copy link
Copy Markdown
Contributor Author

done in 62faa8b.

  • dropped the agent-file entries from the table and renamed the column to "Command directory" — it now lists only what each integration actually manages (its command/skills dir), not the agent-context file. that removes the kiro-cli/codex both-map-to-AGENTS.md contradiction Copilot flagged.
  • reworded the definition to say the agent-context extension is a separate concern and is not multi-install safe (several agents can map to the same context file).
  • removed the duplicate test — test_declares_multi_install_safe already covers it.

on calling it out in the agent-context extension's README: happy to do that as a small follow-up PR so this one stays focused. want me to?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Medium

Comment thread docs/reference/integrations.md Outdated
Comment thread docs/reference/integrations.md Outdated
…rgeting

- Restore 'static, unique agent root' alongside command directory in the
  multi-install-safe definition — base.py and test_registry.py
  (test_safe_integrations_have_distinct_agent_roots) enforce both.
- Reframe the agent-context note: multi_install_safe is an integration-level
  declaration about command/skill paths, so describe context-file targeting
  as independent of it rather than calling the extension 'not multi-install
  safe'. The extension can even sync multiple anchors via context_files.
@mnriem

mnriem commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback

@Quratulain-bilal

Copy link
Copy Markdown
Contributor Author

Addressed both remaining Copilot threads in 6eaf46a:

  • Agent-root requirement (L253): restored "static, unique agent root" alongside the command directory in the definition. You're right that both are part of the contract — IntegrationBase documents it (base.py:133-139) and test_registry.py enforces it via test_safe_integrations_have_distinct_agent_roots and test_safe_integrations_have_distinct_command_dirs. Dropping the root half would have wrongly classified separate subdirs under a shared root as safe.

  • Agent-context framing (L255): reworded so context-file targeting is described as independent of integration multi-install safety, rather than calling the extension itself "not multi-install safe." multi_install_safe is an integration-level declaration about command/skill paths; the agent-context extension is opt-in and can even synchronize multiple anchors via context_files (extensions/agent-context/README.md), so several agents sharing one context file is expected and orthogonal to the flag.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Medium

@@ -13,7 +13,6 @@

class KiroCliIntegration(MarkdownIntegration):
key = "kiro-cli"
@mnriem

mnriem commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback. If not applicable, please explain why. If Copilot is going in circles please also let me know ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: kiro-cli integration should be declared multi_install_safe

3 participants